Skip to content

Conversation

@yuxianrz
Copy link

@yuxianrz yuxianrz commented Aug 4, 2025

Problem

The webview does not support STS credentials input (sessionToken and roleArn) and endpoint to LSP does not support STS credentials and profiles.

Solution

This is part of #7507 and is built on top of #7797.

  • Add STS credentials input box webview, enabling mfa verification if credentials has assume role with mfa permission
  • Modify AuthUtils and auth2.ts to accommodate new IAM profile type
  • Add stsCache and other sts handlers to connect to LSP

  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@yuxianrz yuxianrz requested a review from a team as a code owner August 4, 2025 17:47
@yuxianrz yuxianrz force-pushed the feature/sts-login branch from b7bb5cd to 0ef9118 Compare August 4, 2025 18:05
@yuxianrz yuxianrz changed the title Feature/sts login feat(auth): add STS credential management and mfa verification Aug 4, 2025
@github-actions
Copy link

github-actions bot commented Aug 4, 2025

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

updateCredentialsParams: {
data: 'credential-data',
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a type exist that we could do: } as ReturnTypeOfLogin so that the actual fields are type checked?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will add this.

sinon.stub(auth2, 'IamLogin').returns(mockIamLoginArn as any)

const response = await auth.loginIam(
'accessKey',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: In the future I'd make these values a bit more unique, like MyAccessKey to reduce confusion since there is also the key which is called accessKey

async updateProfile(opts: { accessKey: string; secretKey: string; sessionToken?: string; roleArn?: string }) {
if (opts.roleArn) {
const sourceProfile = this.profileName + '-source'
await this.lspAuth.updateIamProfile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe theres a reason for this, but the interface of updateiamProfile() is a bit awkward. We probably want to use a single object as the argument here since this function takes 6 arguments. Usually after 3 args to a function, I like to make a single argument which is a typed object. Additionally, it will save you from explicitly needing to set '' for the empty fields

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. We are wrapping this into a typed object.

* Entered token is passed to the callback.
* If user cancels out, the callback is passed an error with a fixed message string.
*
* @param profileName Name of Credentials profile we are asking an MFA Token for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These params look out of date

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

}

const defaultCacheDir = () => path.join(fs.getUserHomeDir(), '.aws/sso/cache')
const defaultStsCacheDir = () => path.join(fs.getUserHomeDir(), '.aws/cli/cache')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just verifying that this is the path that the AWS CLI uses for its cache as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are deleting all sts cache watcher logic in the PR as it is not working as intended. This should not be here anymore.

accessKey: string,
secretKey: string,
sessionToken?: string
sessionToken?: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, may be worth to refactor in to a single object as the argument

profileName: string,
accessKey: string,
secretKey: string
secretKey: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above with an object as the argument

@liramon1 liramon1 force-pushed the feature/sts-login branch from e79313c to 8342872 Compare August 5, 2025 16:38
@bywang56 bywang56 merged commit 255214c into aws:feature/flare-mega Aug 6, 2025
16 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants